-
Notifications
You must be signed in to change notification settings - Fork 202
chore: harden recently updated scripts #3680
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
6805649
to
f4571b3
Compare
File metricsSummaryTotal size: 2.25 MB* 🎉 No changes detected in any packages * Size is the sum of all main files for packages in the library.* An ASCII character in UTF-8 is 8 bits or 1 byte. |
🚀 Deployed on https://pr-3680--spectrum-css.netlify.app |
@@ -137,6 +137,8 @@ jobs: | |||
styles_modified_files: ${{ needs.changed_files.outputs.styles_modified_files }} | |||
eslint_added_files: ${{ needs.changed_files.outputs.eslint_added_files }} | |||
eslint_modified_files: ${{ needs.changed_files.outputs.eslint_modified_files }} | |||
mdlint_added_files: ${{ needs.changed_files.outputs.mdlint_added_files }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These exports don't seem to be working so we're linking just all the files!
@@ -29,16 +29,31 @@ body { | |||
} | |||
|
|||
.spectrum { | |||
/* Gradient that changes with the color theme. */ | |||
--spectrum-examples-gradient: linear-gradient(45deg, var(--spectrum-magenta-1500), var(--spectrum-blue-1500)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These variables are specific to the Storybook so I think it makes sense to move their definitions here instead of inside the tokens package. I didn't remove them though in case downstream customers are leveraging them as well.
color: var(--spectrum-neutral-content-color-default); | ||
background-color: var(--spectrum-background-base-color); | ||
-webkit-tap-highlight-color: rgba(0, 0, 0, 0%); | ||
-webkit-tap-highlight-color: rgb(0, 0, 0, 0%); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was just an auto-fix ala VSCode stylelinting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you say more about this and the other change below from hsla to hsl? I would have thought it'd be more correct to use rgba and hsla, but it seems like that's not the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just learned about this the other day too! Apparently rgba and hsla are actually just aliases for rgb and hsl! PostCSS preset env tooling suggests we use the color function directly instead of the alias.
# Legacy tools folder (included storybook & generator) | ||
# test -d "tools" && rm -rf tools | ||
|
||
test -d "tools" && rm -rf tools |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why this is still here - we like and need the tools folder!!
@@ -9,7 +9,7 @@ export const format = ({ dictionary, platform, file, options }) => { | |||
); | |||
|
|||
const convertRef = (ref) => { | |||
return ref.replace(/\{(.*?)\}/g, `var(--${prefix}-$1)`); | |||
return ref?.toString()?.replace(/\{(.*?)\}/g, `var(--${prefix}-$1)`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes ref is a number so this converts it to a string so we can tidy it up first
@@ -23,11 +23,11 @@ export const format = ({ dictionary, platform, file, options }) => { | |||
if (data.ref) { | |||
data.ref = convertRef(data.ref); | |||
|
|||
if (data.ref === data.value) { | |||
if (data.ref?.toString() === data.value?.toString()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just ensures that the ref is converted to the same type before comparing (sometimes they're numbers).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues here! Just left a small question. Also, thanks so much for all the helpful comments you added in the PR!
color: var(--spectrum-neutral-content-color-default); | ||
background-color: var(--spectrum-background-base-color); | ||
-webkit-tap-highlight-color: rgba(0, 0, 0, 0%); | ||
-webkit-tap-highlight-color: rgb(0, 0, 0, 0%); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you say more about this and the other change below from hsla to hsl? I would have thought it'd be more correct to use rgba and hsla, but it seems like that's not the case.
Description
Hind-sight is 20:20! I found a couple super minor updates to the scripts I recently worked on for the style-dictionary and Windows compatibility effort. I added notes inline to the code to help clarify these updates. It should just ensure we get more predictable results.
How and where has this been tested?
Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.
Validation steps
yarn builder tokens
(@rise-erpelding )Regression testing
Validate:
To-do list